@W-18706176: Changes to download latest release version of the API#231
@W-18706176: Changes to download latest release version of the API#231
Conversation
|
Thanks for this @unandyala . The changes have ensured that I download the latest stable oas specs. I wonder if we want to have a toggle where we can disable the check? Just wondering if pulling a snapshot spec for testing purposes is a valid use case we have to account for. |
I don't think we need other versions. There will be lot of them and they are mainly developer versions. They may not even have backend implementation |
joeluong-sfcc
left a comment
There was a problem hiding this comment.
This is a breaking change correct? Other than the minor comments, this PR LGTM, but could you update the CHANGELOG for this change?
| asset.id = "893f605e-10e2-423a-bdb4-f952f56eb6d8/shopper-customers/0.5.0"; | ||
| asset.version = "0.5.0"; | ||
| asset.fatRaml.externalLink = "https://somewhere/fatraml.zip"; | ||
| it("searches Exchange and returns multiple version groupd", () => { |
There was a problem hiding this comment.
| it("searches Exchange and returns multiple version groupd", () => { | |
| it("searches Exchange and returns multiple version groups", () => { |
src/download/exchangeDownloader.ts
Outdated
| return releaseSemverRegex.test(version.version); | ||
| }); | ||
| // Sort versions and get the latest | ||
| return releaseAssetVersions.sort((instanceA, instanceB) => { |
There was a problem hiding this comment.
This function is a nice solution, it feels like a very clean way to get the latest version, straightforward to understand
src/download/exchangeDownloader.ts
Outdated
| ramlToolLogger.warn(DEPLOYMENT_DEPRECATION_WARNING); | ||
| } | ||
|
|
||
| export async function search(query: string): Promise<RestApi[]> { |
There was a problem hiding this comment.
We're planning to search based on the API SDK Type and Visibility categories/types correct? Previously we queried by name... How does it look now when we call search function in the SDKs?
There was a problem hiding this comment.
For node sdk, it will be
await download.search( 'category:Visibility = "External" category:"SDK Type" = "Commerce"' );
For isomorphic sdk, it will be
await download.search( 'category:Visibility = "External" category:"SDK Type" = "Commerce"' category:"SDK Type" = "Isomorphic"' );
src/download/exchangeDownloader.ts
Outdated
| * @returns {Promise<string>} Returned the version string from the instance fetched asset.version value | ||
| */ | ||
| export async function getVersionByDeployment( | ||
| export async function getApiVersions( |
There was a problem hiding this comment.
It's not clear in the description or the function name that we get the latest version, we should rename this function to something like getLatestApiVersions and/or update the JS doc description
There was a problem hiding this comment.
Since getApiVersions is a new function, can we keep the old getVersionByDeployment still?
Would keeping getVersionByDeployment make this a non-breaking change?
Does raml-toolkit have other users aside from commerce-sdk / commerce-sdk-isomorphic? This change would break those, if they exists.
Also, if we're going to be making a breaking change here, are there other breaking changes we also want to get in?
There was a problem hiding this comment.
We marked deployment flag as deprecated a while back. So I think it is ok to change the function name.
The raml-toolkit is only used by commerce-sdk / commerce-sdk-isomorphic
There was a problem hiding this comment.
Refactored to put all the new code behind isOas flag
src/download/exchangeDownloader.ts
Outdated
|
|
||
| return Promise.all(promises).then((results) => | ||
| results.reduce((acc, val) => acc.concat(val), []) | ||
| ); |
There was a problem hiding this comment.
This isn't immediately clear to me what this does, could you add a small comment?
vcua-mobify
left a comment
There was a problem hiding this comment.
Thanks for refactoring this PR to be a non-breaking change! Good idea to use the isOas to feature flag all of the changes.
LGTM
| chai.use(chaiAsPromised); | ||
| }); | ||
|
|
||
| describe("extractFile", () => { |
There was a problem hiding this comment.
Thanks for adding this. From the looks of it, we were only testing extractFiles before and not extractFile.
| ramlToolLogger.error( | ||
| `${logPrefix} The rest API ${restApi.assetId} is missing asset.versionGroups` | ||
| ); | ||
| return; |
There was a problem hiding this comment.
Probably a good idea to give the API team a heads up about this since I don't know if everything defines a versionGroup.
joeluong-sfcc
left a comment
There was a problem hiding this comment.
LGTM, only thing has the CI always failed since we added OAS support in RAML toolkit, or did it start with this PR? Either way we should resolve the CI so that tests pass
this is an existing issue not related to the PR - /bin/sh: 1: oasdiff: not found |
18916b8
vcua-mobify
left a comment
There was a problem hiding this comment.
Thanks for fixing the CI issue @unandyala
@W-18706176 @W-18763414
Testing
download command
node bin/run download --dest=raml-test --search='category:Visibility = "External"' --log-level=debugran updateApis script in commerce-isomorphic-sdk